Skip to content

Fix explicit_counter_loop suggestion #3901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 8, 2019
Merged

Fix explicit_counter_loop suggestion #3901

merged 3 commits into from
Apr 8, 2019

Conversation

rail-rain
Copy link
Contributor

#1670

This code seems to me to work, but I have two question.

  • Because range expression desugared in hir, Sugg::hir doesn't add parenthesis to range expression. Which function is better to check range do you think, check_for_loop_explicit_counter or hir_from_snippet?
  • Do you think we need to distinguish between range expression and struct expression that creates std::ops::Range*?

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 23, 2019
@flip1995
Copy link
Member

flip1995 commented Mar 25, 2019 •
edited
Loading

Thanks for the contribution and welcome to Clippy!

This LGTM. For suggestions we use the function:

rust-clippy/clippy_lints/src/utils/diagnostics.rs

Lines 169 to 177 in 61aa5c9

pub fn span_lint_and_sugg<'a, 'tcx: 'a, T: LintContext<'tcx>>(
cx: &'a T,
lint: &'static Lint,
sp: Span,
msg: &str,
help: &str,
sugg: String,
applicability: Applicability,
) {

You have to split up the error message into the message and the suggestion.

PS: The currently failing CI is unrelated: #3897

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 25, 2019
flip1995
flip1995 approved these changes Mar 28, 2019
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now.

Since the CI/Clippy master is currently broken, could you check if you have to run rustfmt?

Otherwise, just the additional test case should be added and this can be merged.

@rail-rain
Copy link
Contributor Author

I runned rustfmt before committing and I checked this by rustfmt 1.1.0-nightly (1427e4c2 2019-03-17), but how do I assure you it?

@flip1995
Copy link
Member

flip1995 commented Mar 29, 2019 •
edited
Loading

All good, just wanted to remind you, so that I can just merge this and don't have to bother you, once Clippy builds again.

@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 29, 2019
@rail-rain rail-rain changed the title WIP - Fix explicit_counter_loop suggestion Fix explicit_counter_loop suggestion Mar 30, 2019
@phansch
Copy link
Contributor

phansch commented Apr 8, 2019

@bors r=flip1995

@bors
Copy link
Contributor

bors commented Apr 8, 2019

📌 Commit 2b82c71 has been approved by flip1995

bors added a commit that referenced this pull request Apr 8, 2019
@bors
Auto merge of #3901 - rail-rain:issue_1670, r=flip1995
42e1cf3 
Fix `explicit_counter_loop` suggestion

#1670

This code seems to me to work, but I have two question.
* Because range expression desugared in hir, `Sugg::hir` doesn't add parenthesis to range expression.  Which function is better to check range do you think, `check_for_loop_explicit_counter` or `hir_from_snippet`?
* Do you think we need to distinguish between range expression and struct expression that creates `std::ops::Range*`?
@bors
Copy link
Contributor

bors commented Apr 8, 2019

⌛ Testing commit 2b82c71 with merge 42e1cf3...

@bors
Copy link
Contributor

bors commented Apr 8, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 42e1cf3 to master...

@bors bors merged commit 2b82c71 into rust-lang:master Apr 8, 2019
@rail-rain rail-rain mentioned this pull request Oct 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rail-rain @flip1995 @phansch @bors